chore(notification-center,node,shared): update the axios to latest version #3861
Conversation
NV-2614 nodejs sdk is returning binary data response.
What?On using nodejs sdk methods in express project, it returns binary data response like It works perfect in nestjs project Why? (Context)novu/node has a dependency on novu/shared which uses an old version of axios. 1.2.0 Definition of Donenode SDK should not return this binary response in express project |
| @@ -26,7 +26,7 @@ | |||
| "dist/" | |||
| ], | |||
| "dependencies": { | |||
| "axios": "1.2.0", | |||
| "axios": "^1.4.0", | |||
There was a problem hiding this comment.
the Axios version 1.2.0 was buggy, updating it to the latest helped solve the original issue, but...
| "build": "npm run build:cjs && npm run build:esm && npm run build:umd && npm run build:types", | ||
| "build:cjs": "cross-env node_modules/.bin/tsc -p tsconfig.json", | ||
| "build:esm": "cross-env node_modules/.bin/tsc -p tsconfig.esm.json", | ||
| "build:esm:watch": "cross-env node_modules/.bin/tsc -p tsconfig.esm.json -w --preserveWatchOutput", | ||
| "build:umd": "webpack --config webpack.config.js", | ||
| "build:types": "tsc --declaration --emitDeclarationOnly --declarationMap --declarationDir dist/types -p tsconfig.json", | ||
| "build:watch": "rollup -c -w", | ||
| "build:watch": "npm run build:esm:watch", |
There was a problem hiding this comment.
That latest Axios version broke the UMD build of the notification-center package.
First of all, Rollup was bundling the Axios for node env, which required to polyfill NodeJS native modules like http, utils, ... etc.
The second thing was that the Rollup NodeJS polyfills do not support everything that Axios is using right now in Axios for node.
I wasn't able to figure out why it was trying to bundle the Axios for node, but I was sure that it was the issue between rollup plugins.
After a long fight, I decided to give it a try to Webpack, and in a few minutes, I had it working like a charm.
Then CJS and ESM modules needed to be built with TSC, but we were importing some images in the components, so I had to fix them too, like to make them React SVG components.
| @@ -2,10 +2,10 @@ import React from 'react'; | |||
| import { IMessage, ChannelCTATypeEnum } from '@novu/shared'; | |||
|
|
|||
| import { useNotifications, useNotificationCenter, useNovuContext, useTranslations } from '../../../hooks'; | |||
| import image from '../../../images/no-new-notifications.svg'; | |||
There was a problem hiding this comment.
I've transformed svg and webp images to React SVG components
| { | ||
| "extends": "./tsconfig.json", | ||
| "compilerOptions": { | ||
| "module": "ESNext", | ||
| "outDir": "./dist/esm" | ||
| } | ||
| } |
There was a problem hiding this comment.
separate TS config for the ESM module
| "forceConsistentCasingInFileNames": true, | ||
| "target": "es6", | ||
| "strict": true, | ||
| "typeRoots": ["./node_modules/@types"], | ||
| "jsx": "react", | ||
| "module": "ESNext", | ||
| "module": "commonjs", |
There was a problem hiding this comment.
TS config for CJS
What change does this PR introduce?
Fixes the original issue from the ticket and additionally:
notification-centerbundle config: use webpack for umd, tsc for esm and cjsVerified if the
@novu/nodepackage works in:Verified if the
@novu/notification-centerpackage works in:Verified if the
@novu/notification-center-vueand@novu/notification-center-angularpackages works.Why was this change needed?
Other information (Screenshots)